Skip to content

✨ build improvements, test updates & agentic refactoring [1/3#1388

Closed
ibolton336 wants to merge 3 commits intokonveyor:mainfrom
ibolton336:refactor/state-management-infra
Closed

✨ build improvements, test updates & agentic refactoring [1/3#1388
ibolton336 wants to merge 3 commits intokonveyor:mainfrom
ibolton336:refactor/state-management-infra

Conversation

@ibolton336
Copy link
Copy Markdown
Member

@ibolton336 ibolton336 commented May 1, 2026

Summary

Agentic package improvements, LLM reliability fixes, and build infrastructure for the pluggable agent backend. No runtime behavior changes to the VS Code extension UI — safe to merge independently.

What's included

Agentic — LLM timeout protection (base.ts):

  • Adds a configurable 5-minute timeout on all LLM requests via Promise.race
  • Previously, a misconfigured or unreachable model provider would hang streamOrInvoke indefinitely
  • Now throws a clear error: "Verify your provider-settings.yaml configuration is correct"
  • Configurable per-call via timeoutMs option

Agentic — Error emission fix (base.ts):

  • Re-gates error workflow message emission on the emitResponseChunks flag
  • Internal "thinking" operations (summarizeHistory, planFixes, summarizeAdditionalInformation) that set emitResponseChunks: false no longer leak error messages to the user-facing workflow stream

Agentic — Better no-response logging:

  • analysisIssueFix.ts: AnalysisIssueFix and SummarizeHistorylogger.silly()logger.warn() with "This may indicate a model provider configuration issue"
  • diagnosticsIssueFix.ts: PlanFixes, FixGeneralIssues, FixJavaDependencyIssues — same pattern

Agentic — Type additions (types.ts):

  • Added originalContent?: string to KaiModifiedFile — needed by PR Add barebones react app to webview #2's Goose agent flow where files are written to disk before processing (used for diffing/revert)
  • Added result?: string to KaiToolCall — allows storing tool call results

Agentic — Formatting (cache.ts, types.ts):

  • implements clause line wrapping for FileBasedResponseCache and InMemoryCacheWithRevisions
  • Trailing comma on KaiWorkflow generic parameter

Build infrastructure:

  • package.json — Added mcp-server workspace, updated postinstall and dev scripts
  • package-lock.json — Updated for new workspace
  • .husky/pre-commit — NVM setup with ${NVM_DIR:-$HOME/.nvm} fallback for non-standard installs

Changelog:

  • 1351-pluggable-agent-backend.yaml — Feature entry for pluggable agent backend
  • fix-duplicate-loading-indicators.yaml — Bugfix entry for duplicate loading indicators

Stack order

This is PR 1 of 3 breaking up the feature/goose branch:

  1. → Agentic improvements & build infrastructure (this PR)
  2. Pluggable agent backend → ✨ pluggable agent backend — Goose, OpenCode, MCP server, orchestrator + chat panel UI #1389 (depends on this)
  3. Chat panel UI → depends on Add barebones react app to webview #2

Testing

  • No VS Code extension runtime changes — existing tests unaffected
  • Build verified locally ✅

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 1, 2026

Warning

Rate limit exceeded

@ibolton336 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 28 minutes and 56 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9114aa69-ea70-45dd-b654-243db40f7854

📥 Commits

Reviewing files that changed from the base of the PR and between 6eb0ff7 and 9b1d449.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (8)
  • .husky/pre-commit
  • agentic/src/cache.ts
  • agentic/src/nodes/analysisIssueFix.ts
  • agentic/src/nodes/base.ts
  • agentic/src/nodes/diagnosticsIssueFix.ts
  • agentic/src/types.ts
  • changes/unreleased/1351-pluggable-agent-backend.yaml
  • changes/unreleased/fix-duplicate-loading-indicators.yaml
📝 Walkthrough

Walkthrough

This pull request makes systemic changes across build configuration, language server integration, asset management, and test infrastructure. It removes the konveyor-analyzer-dep asset dependency from the Go extension packaging and collection scripts; switches the Go provider subprocess invocation to use a generic name instead of extension-specific naming; eliminates Java extension Standard-mode readiness checks and defers initialization directly after extension activation; introduces a new mcp-server npm workspace with corresponding build and dev scripts; adds timeout handling and error-emission improvements to the base LLM invocation logic; refactors E2E test page automation for workspace/extension handling and fixture provider configuration; removes branding-related environment variables (TEST_CATEGORY, TEST_CATEGORY_LONG) from test configuration; and extends workflow message types with optional fields for content preservation and tool results.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested reviewers

  • djzager
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title mentions "build improvements, test updates & agentic refactoring" which aligns with the PR's main changes across CI workflows, test infrastructure, agentic package refactoring, and build scripts. However, the title is incomplete (ends with "[1/3") and uses an emoji, which adds noise.
Description check ✅ Passed The pull request description includes a PR title prefix, detailed summary of changes, implementation details, build infrastructure updates, and testing notes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 28 minutes and 56 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
.husky/pre-commit (1)

5-6: ⚡ Quick win

Respect existing NVM_DIR for better portability.

The current implementation unconditionally sets NVM_DIR to $HOME/.nvm, which assumes the default installation location and overrides any existing environment variable. Developers with non-standard NVM installations (e.g., via Homebrew or custom paths) may encounter issues.

🔧 Proposed improvement to respect existing NVM_DIR
-export NVM_DIR="$HOME/.nvm"
+export NVM_DIR="${NVM_DIR:-$HOME/.nvm}"
 [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"

This change uses parameter expansion to respect an existing NVM_DIR while providing $HOME/.nvm as a fallback.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.husky/pre-commit around lines 5 - 6, The pre-commit hook unconditionally
overwrites NVM_DIR which breaks non-standard NVM installs; update the
.husky/pre-commit script so it preserves an existing NVM_DIR and only falls back
to $HOME/.nvm when undefined (use shell parameter expansion for NVM_DIR), then
source "$NVM_DIR/nvm.sh" as before; reference the NVM_DIR variable and the
existing source line to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@agentic/src/nodes/base.ts`:
- Around line 159-163: The code currently always emits an error workflow message
via emitWorkflowMessage with type KaiWorkflowMessageType.Error regardless of the
emitResponseChunks flag; update the error emission in the function/method that
calls emitWorkflowMessage so that it checks the existing emitResponseChunks
boolean (or a new emitErrors option) before sending errors to the workflow
stream—i.e., wrap the emitWorkflowMessage(...) error call in if
(emitResponseChunks) { ... } or if (emitResponseChunks || emitErrors) { ... }
and propagate/define emitErrors where needed (caller sites like
summarizeAdditionalInformation, summarizeHistory, planFixes) so internal
"thinking" operations that set emitResponseChunks:false do not surface errors
unless explicitly allowed.

In `@tests/e2e/enums/output.enum.ts`:
- Around line 1-4: The test enum forces a single exact label for CoreExtension
which breaks branded variants; update the OutputChannels handling so tests
accept both formats instead of a single string — e.g. return or export a
matcher/array that includes both `${extensionShortName} Core Extension` and the
legacy/long-name variant (or switch test lookup to use startsWith/contains
against extensionShortName). Modify the exported OutputChannels.CoreExtension
usage (and any tests that import it) to use the new flexible matcher/array so
E2E channel selection is robust across branded builds.

In `@vscode/go/src/goExternalProviderManager.ts`:
- Line 53: The spawn invocation that starts the external provider (this.process
= spawn(...)) passes an undocumented "-name generic" flag; remove the "-name"
argument and only pass supported flags (e.g., "-socket",
this.providerSocketPath), and instead ensure the provider identification is set
in provider_settings.json (name: "generic") or via the provider configuration
API used by go-external-provider; update the code that constructs the argument
array in goExternalProviderManager (the spawn call) to match the binary's
documented CLI and add/verify provider_settings.json creation or loading logic
so the provider name is configured there.

---

Nitpick comments:
In @.husky/pre-commit:
- Around line 5-6: The pre-commit hook unconditionally overwrites NVM_DIR which
breaks non-standard NVM installs; update the .husky/pre-commit script so it
preserves an existing NVM_DIR and only falls back to $HOME/.nvm when undefined
(use shell parameter expansion for NVM_DIR), then source "$NVM_DIR/nvm.sh" as
before; reference the NVM_DIR variable and the existing source line to locate
and modify the code.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f10b0d52-f0b9-4cfe-b824-fa0e676a47ad

📥 Commits

Reviewing files that changed from the base of the PR and between 3d97606 and 6eb0ff7.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (33)
  • .github/workflows/e2e-tests.yml
  • .husky/pre-commit
  • agentic/src/cache.ts
  • agentic/src/nodes/analysisIssueFix.ts
  • agentic/src/nodes/base.ts
  • agentic/src/nodes/diagnosticsIssueFix.ts
  • agentic/src/types.ts
  • changes/unreleased/1343-ignore-non-workspace-files.yaml
  • changes/unreleased/1349-java-retry-activation.yaml
  • changes/unreleased/1351-pluggable-agent-backend.yaml
  • changes/unreleased/1374-add-enavled-property-to-llmproxy-client.yaml
  • changes/unreleased/fix-duplicate-loading-indicators.yaml
  • changes/unreleased/fix-review-mode-continue-button-blocked.yaml
  • changes/unreleased/fix-solution-server-inline-edit-capture.yaml
  • package.json
  • scripts/collect-assets.js
  • scripts/copy-dist.js
  • tests/.env.example
  • tests/docs/contrib/e2e-environment.md
  • tests/e2e/enums/output.enum.ts
  • tests/e2e/fixtures/provider-configs.fixture.ts
  • tests/e2e/fixtures/test-repos.json
  • tests/e2e/pages/vscode-web.page.ts
  • tests/e2e/pages/vscode.page.ts
  • tests/e2e/tests/base/plugin-settings.test.ts
  • tests/e2e/tests/ccm/llm-proxy.test.ts
  • tests/e2e/utilities/utils.ts
  • vscode/core/src/utilities/ModifiedFiles/handleFileResponse.ts
  • vscode/go/package.json
  • vscode/go/src/extension.ts
  • vscode/go/src/goExternalProviderManager.ts
  • vscode/go/src/pathUtils.ts
  • vscode/java/src/extension.ts
💤 Files with no reviewable changes (11)
  • changes/unreleased/1343-ignore-non-workspace-files.yaml
  • tests/e2e/tests/ccm/llm-proxy.test.ts
  • changes/unreleased/1349-java-retry-activation.yaml
  • changes/unreleased/fix-solution-server-inline-edit-capture.yaml
  • changes/unreleased/1374-add-enavled-property-to-llmproxy-client.yaml
  • tests/e2e/utilities/utils.ts
  • tests/.env.example
  • tests/docs/contrib/e2e-environment.md
  • scripts/copy-dist.js
  • changes/unreleased/fix-review-mode-continue-button-blocked.yaml
  • vscode/go/src/pathUtils.ts

Comment thread agentic/src/nodes/base.ts Outdated
Comment thread tests/e2e/enums/output.enum.ts Outdated
Comment thread vscode/go/src/goExternalProviderManager.ts Outdated
@ibolton336 ibolton336 force-pushed the refactor/state-management-infra branch from 6eb0ff7 to 7a4c368 Compare May 1, 2026 16:23
Copy link
Copy Markdown
Member Author

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — looks good to merge. Clean foundation layer for the pluggable agent backend stack.

CodeRabbit items addressed (pushed fix commit):

  • .husky/pre-commit — now respects existing NVM_DIR via parameter expansion fallback (${NVM_DIR:-$HOME/.nvm})
  • agentic/src/nodes/base.ts — error emission gated on emitResponseChunks again, so internal "thinking" operations (summarizeHistory, planFixes, etc.) don't leak errors to the workflow stream

CodeRabbit items NOT applicable:

  • tests/e2e/enums/output.enum.ts — the branch already has the branded variant conditional logic; the CodeRabbit comment was about a diff artifact
  • vscode/go/src/goExternalProviderManager.ts — the branch already uses -name go (not generic); same diff artifact issue

Build improvements, agentic refactoring, and test fixture updates all look good. No runtime behavior changes to the VS Code extension.

@ibolton336 ibolton336 force-pushed the refactor/state-management-infra branch 2 times, most recently from 3ef6f39 to ab29ff5 Compare May 1, 2026 16:29
Agentic package improvements and LLM reliability fixes. No runtime
behavior changes to the VS Code extension UI — safe to merge independently.

Removed collect-assets, provider, husky, go/java extension, and build
changes that overlap with konveyor#1379.

### What's included
- **Agentic package** — refactored base node with 5-min LLM timeout,
  improved caching, updated types (originalContent, result fields)
- **LLM reliability** — warn-level logging when LLM returns no response,
  timeout protection against hanging requests
- **Changelog** — added new fragments for pluggable agent backend and
  duplicate loading indicator fix

### Stack order
This is **PR 1 of 3** breaking up the feature/goose branch:
1. **→ Agentic refactoring & LLM improvements** (this PR)
2. Pluggable agent backend → depends on this
3. Chat panel UI → depends on konveyor#2

Signed-off-by: ibolton336 <ibolton@redhat.com>
@ibolton336 ibolton336 force-pushed the refactor/state-management-infra branch from ab29ff5 to 9675cfa Compare May 1, 2026 16:31
@ibolton336 ibolton336 changed the title ✨ build improvements, test updates & agentic refactoring [1/3] ✨ build improvements, test updates & agentic refactoring [1/3 May 1, 2026
@ibolton336 ibolton336 force-pushed the refactor/state-management-infra branch 3 times, most recently from a754b74 to 557b037 Compare May 1, 2026 16:33
- husky pre-commit: respect existing NVM_DIR with parameter expansion fallback
- base.ts: gate error emission on emitResponseChunks to avoid surfacing
  internal "thinking" operation errors to users

Signed-off-by: ibolton336 <ibolton@redhat.com>
@ibolton336 ibolton336 force-pushed the refactor/state-management-infra branch from 557b037 to 90ec75a Compare May 1, 2026 16:34
The mcp-server workspace is introduced in a later PR in the stack.
Referencing it in postinstall breaks `npm ci` in CI.

Signed-off-by: Ian Bolton <ibolton@redhat.com>
@ibolton336
Copy link
Copy Markdown
Member Author

Closing — all changes in this PR are already included in #1389 (every file diff is a subset). The split didn't add enough value to justify a separate PR, so consolidating into #1389 as a single PR.

@ibolton336 ibolton336 closed this May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant